Skip to content

Handle pg_dump restrict commands in local restore#611

Closed
jirhiker wants to merge 2 commits intostagingfrom
local-db-restore
Closed

Handle pg_dump restrict commands in local restore#611
jirhiker wants to merge 2 commits intostagingfrom
local-db-restore

Conversation

@jirhiker
Copy link
Copy Markdown
Member

Summary

  • strip pg_dump \restrict and \unrestrict meta-commands from staged restore SQL before invoking local psql
  • keep returning the staged SQL path in the restore result so successful restores do not fail constructing the result object
  • add CLI test coverage for restores from dumps that include the new pg_dump meta-commands

Testing

  • source .venv/bin/activate && pytest tests/test_cli_commands.py -k restore_local_db

Copilot AI review requested due to automatic review settings March 19, 2026 16:27
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the local database restore flow to be compatible with newer pg_dump output by removing psql-only \restrict/\unrestrict meta-commands during staging, and fixes the restore result construction while adding CLI test coverage for the new sanitization behavior.

Changes:

  • Strip \restrict and \unrestrict lines (in addition to existing role-dependent statements) from staged SQL before invoking psql.
  • Ensure LocalDbRestoreResult construction includes the staged SQL path (sql_file).
  • Extend CLI restore test to cover dumps that include the new meta-commands.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
cli/db_restore.py Adds sanitization for pg_dump meta-commands and fixes LocalDbRestoreResult construction.
tests/test_cli_commands.py Updates restore CLI test input to include \restrict/\unrestrict and asserts they’re removed.

You can also share your feedback on Copilot code review. Take the survey.

Comment on lines 247 to +248
return LocalDbRestoreResult(
sql_file=staged_sql_file,
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 20aafdd21d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +93 to +96
matches_psql_meta = any(
pattern.search(line) for pattern in PSQL_META_COMMAND_PATTERNS
)
if matches_role_sql or matches_psql_meta:
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Do not strip pg_dump's \restrict safety guard

PostgreSQL 17.6 added \restrict specifically to block injected psql meta-commands during plain-text restores (see the 17.6 release notes for CVE-2025-8714, and app-psql's note that restricted mode only allows \unrestrict). Because this sanitizer removes those lines and then still feeds the file to psql -f, any dump restored from GCS or another compromised source can once again execute meta-commands such as \! on the operator's machine. For older local clients we should fail fast or require a newer psql, not silently discard the protection.

Useful? React with 👍 / 👎.

Comment on lines 247 to +248
return LocalDbRestoreResult(
sql_file=staged_sql_file,
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Avoid returning a staged SQL path that is already deleted

result.sql_file now points at staged_sql_file, but _stage_restore_source() creates that file inside a TemporaryDirectory() and removes it before restore_local_db_from_sql() returns. Any caller that tries to inspect or reuse result.sql_file after a successful restore will immediately hit a missing file, so the new field does not actually provide the staged SQL path advertised by LocalDbRestoreResult.

Useful? React with 👍 / 👎.

@jirhiker jirhiker marked this pull request as draft March 19, 2026 16:35
@jirhiker jirhiker closed this Mar 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants